RegisterResponder API by msnidhin · Pull Request #98 · CodeConstruct/mctp

msnidhin · 2025-07-25T06:52:13Z

GetMsgType response is hardcoded and always return only control
message support. Add a dbus method RegisterTypeSupport which accept msg
type and versions array as input and reply accordingly in GetMsgType and
GetVersin control commands

Tested by sending GetMsgType and GetVersion from another EP
Default responses for GetMsgType and GetVersion for type 0
5, 0, 1, 0 and 4, 0, 4, 241, 240, 255, 0, 241, 241, 255, 0, 241, 242, 255, 0, 241, 243, 241, 0

After calling
au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 au.com.codeconstruct.MCTP1 RegisterTypeSupport yau 5 1 0xF1F2F3F4
with a tool which stays alive after method call
Response for GetMsgType and GetVersion for type 5
5, 0, 2, 0, 5 and 4, 0, 1, 244, 243, 242, 241

jk-ozlabs

Thanks for the contribution. A couple of comments inline, and can you add some tests too?

jk-ozlabs · 2025-07-25T06:56:47Z

+	size_t count;
+
+	// Assume array index follows same order. Msg type at msg_types[0]
+	// will have version_counts[0] versions stored at versions[0]


Better to use an array of structs for this, rather than three separate arrays. You'll probably find that this simplifies the registration and setup too.

You probably don't need this parent struct, as you only ever create one of them. All of the other counted arrays use that style.

I thought associating version with msg type will be easy with this parent struct
Like

struct msg_type_support { uint8_t type; size_t num_versions; uint32_t *versions; }; struct msg_type_support* supported_msg_types; size_t num_msg_types; // Use like supported_msg_types[0].type; supported_msg_types[0].versions;

Yep, that looks better.

This is changed now

jk-ozlabs · 2025-07-25T06:58:09Z

+	uint32_t *ctrl_cmd_versions = NULL;
+	ctx->supported_msg_types.msg_types = NULL;
+	ctx->supported_msg_types.versions = NULL;
+	ctx->supported_msg_types.version_counts = NULL;
+
 	ctx->mctp_timeout = 250000; // 250ms
 	ctx->default_role = ENDPOINT_ROLE_BUS_OWNER;
+	ctx->supported_msg_types.count = 0;
+
+	// Default to supporting only control messages
+	ctx->supported_msg_types.msg_types = malloc(1);
+	if (!ctx->supported_msg_types.msg_types) {
+		warnx("Out of memory for supported message types");
+		goto oom_err;
+	}
+	ctrl_cmd_versions = malloc(sizeof(uint32_t) * 4);
+	if (!ctrl_cmd_versions) {
+		warnx("Out of memory for versions");
+		goto oom_err;
+	}
+	ctx->supported_msg_types.versions = malloc(sizeof(uint32_t *));
+	if (!ctx->supported_msg_types.versions) {
+		warnx("Out of memory for versions");
+		goto oom_err;
+	}
+	ctx->supported_msg_types.version_counts = malloc(sizeof(size_t));
+	if (!ctx->supported_msg_types.version_counts) {
+		warnx("Out of memory for version counts");
+		goto oom_err;
+	}
+
+	ctx->supported_msg_types.count = 1;
+	ctx->supported_msg_types.msg_types[0] = MCTP_CTRL_HDR_MSG_TYPE;
+	ctrl_cmd_versions[0] = htonl(0xF1F0FF00);
+	ctrl_cmd_versions[1] = htonl(0xF1F1FF00);
+	ctrl_cmd_versions[2] = htonl(0xF1F2FF00);
+	ctrl_cmd_versions[3] = htonl(0xF1F3F100);
+	ctx->supported_msg_types.versions[0] = ctrl_cmd_versions;
+	ctx->supported_msg_types.version_counts[0] = 4;
+
+	return;
+oom_err:
+	free(ctrl_cmd_versions);
+	free(ctx->supported_msg_types.msg_types);
+	free(ctx->supported_msg_types.versions);
+	free(ctx->supported_msg_types.version_counts);
+	ctx->supported_msg_types.msg_types = NULL;
+	ctx->supported_msg_types.versions = NULL;
+	ctx->supported_msg_types.version_counts = NULL;


These aren't really config options, so this init doesn't belong in the config_defaults setup.

Sure. Can you suggest a better place to put thsese ?

Just where we first set up the struct ctx. I would suggest just a helper function called from main.

Moved to a new function

jk-ozlabs · 2025-07-25T06:58:31Z

+	method_register_responder,
+	0),
+	SD_BUS_VTABLE_END,
+};


Any dbus API changes should be documented in mctpd.md

Also, how are you thinking of handling unregister?

Leaving this to later is fine, we may at least need to accommodate a design for that in the registration function.

I just want to add that if we choose to require the message type lifetime to be bound to a D-Bus peer, existing sd_bus_track API might be used. If D-Bus peer disappears, we can remove the type(s) registered by the peer (which should probably be a daemon of some kinds)

Sounds like a great idea; we would just sd_bus_track_add_sender from the source.

... In which case we do not need any lifetime-management considerations in the proposed API. We would just need to document that the lifetime is managed by mctpd.

Unregister handling is pending. Will push that also.

I'm fine with leaving the unregister handling to a separate PR if you like. All we needed to do at this stage was have some approach in mind, in case that influences the registration mechanism.

msnidhin · 2025-08-01T08:09:42Z

Thanks for the contribution. A couple of comments inline, and can you add some tests too?

Is the test steps in doc still valid ?
I was getting an error in ninja -C obj test command
../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)

jk-ozlabs · 2025-08-05T01:20:43Z

../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)

that's odd, we should get if.h; but I'll add an explicit include. What environment are you compiling this on?

msnidhin · 2025-08-05T09:03:38Z

../src/mctp-netlink.h:37:23: error: ‘IFNAMSIZ’ undeclared here (not in a function)
that's odd, we should get if.h; but I'll add an explicit include. What environment are you compiling this on?

Im compiling from an Ubuntu server system. Ubuntu 22.04 LTS
gcc (Ubuntu 13.1.0-8ubuntu1~22.04) 13.1.0

jk-ozlabs · 2025-08-07T02:45:23Z

(the PR fixing this has been merged, feel free to rebase for testing)

msnidhin · 2025-08-07T04:36:35Z

(the PR fixing this has been merged, feel free to rebase for testing)

Hi. Sorry if this is getting long. Im hit with another issue
../src/mctp-bench.c:322:2: error: #error No ALLOCTAG ioctl available
322 | #error No ALLOCTAG ioctl available

Is there any prerequisites for the environment to run the test? Im trying that two commands only
meson setup obj and ninja -C obj test

jk-ozlabs · 2025-08-07T05:01:27Z

The issue is that the kernel headers you are building against predate a lot of the MCTP support. Even if we get the build working, you are unlikely to be able to actually run any of the binaries produced on that system.

That said, if you want to just run the tests (they're independent of running kernel version), you may be able to work-around the above issue by disabling the mctp-bench build.

Something like:

diff --git a/meson.build b/meson.build
index f93d28c..fc4e0bb 100644
--- a/meson.build
+++ b/meson.build
@@ -71,9 +71,9 @@ executable('mctp-echo',
     sources: ['src/mctp-echo.c'] + util_sources,
 )
 
-executable('mctp-bench',
-    sources: ['src/mctp-bench.c'] + util_sources,
-)
+#executable('mctp-bench',
+#    sources: ['src/mctp-bench.c'] + util_sources,
+#)
 
 executable('mctp-client',
     sources: ['src/mctp-client.c'] + util_sources,

jk-ozlabs · 2025-08-07T05:06:47Z

I see that the original release of 22.04 used 5.15, which would explain those errors. However, there has subsequently been an update to 22.04 that brings it up to 6.8, which would be fine for these builds.

vikash-chandola · 2025-09-02T09:10:20Z

+		warnx("Out of memory for versions");
+		free(ctx->supported_msg_types);
+		return;


It will be good to set num_supported_msg_types to 0 and supported_msg_types to NULL on second malloc failure.

msnidhin · 2025-09-12T03:01:17Z

Added a basic test case for get message type responder. Please revisit

msnidhin · 2025-09-12T06:02:50Z

Hi. tracking the sender is also added. please provide feedback

jk-ozlabs

Mostly looks good, thanks! A few minor changes inline, and I think the commit message is out-of-date now, as we're not doing the type registratons in the config file any more.

jk-ozlabs · 2025-09-12T09:18:59Z

+```
+NAME                                TYPE      SIGNATURE  RESULT/VALUE  FLAGS
+au.com.codeconstruct.MCTP1          interface -          -             -
+.RegisterResponder                  method    yau        -             -


The Responder term is not super well-defined at this level. How about RegisterTypeSupport, which matches the terminology for the Get Message Type Support command in the spec?

jk-ozlabs · 2025-09-12T09:20:26Z

+	} else {
+		ver_count = ctx->supported_msg_types[ver_idx].num_versions;
+		respbuf =
+			malloc(sizeof(*resp) + (ver_count * sizeof(uint32_t)));


Fairly minor, but you calculate ver_count * sizeof(uint32_t) in multiple places here. Maybe use a temporary var?

jk-ozlabs · 2025-09-12T09:21:20Z

-	uint8_t respbuf[sizeof(*resp) + 4 * sizeof(*versions)] = { 0 };
+	uint8_t *respbuf = NULL;
 	size_t resp_len;
+	size_t i, ver_count = 0;


Minor: since you're declaring multiple vars, may as well merge this with the resp_len declaration too.

jk-ozlabs · 2025-09-12T09:23:22Z

+	}
+	ctx->supported_msg_types = msg_types;
+	struct msg_type_support *cur_msg_type =
+		ctx->supported_msg_types + ctx->num_supported_msg_types;


I have a slight preference for array indexing here:

struct msg_type_support *cur_msg_type = &ctx->supported_msg_types[ctx->num_supported_msg_types];

jk-ozlabs · 2025-09-12T09:26:10Z

+			(ctx->num_supported_msg_types + 1) *
+				sizeof(struct msg_type_support));
+	if (!msg_types) {
+		goto oom_err;


No need for a goto for a direct-return

jk-ozlabs · 2025-09-12T09:29:30Z

+	// Extra memory for last msg type will remain allocated but tracked
+	if (cur_msg_type->source_peer)
+		sd_bus_track_unref(cur_msg_type->source_peer);
+	cur_msg_type->source_peer = NULL;


Why set to null? this array entry is unreachable anyway.

Also, sd_bus_track_unref() allows NULL as an argument, so you don't need the conditional.

jk-ozlabs · 2025-09-12T09:31:57Z

+	for (i = 0; i < ctx->num_supported_msg_types; i++) {
+		free(ctx->supported_msg_types[i].versions);
+	}
+	free(ctx->supported_msg_types);


This is no longer symmetrical with parse config, since these are initialised in setup_ctrl_cmd_defaults. We should have a matching free_ function that does this.

added new function

jk-ozlabs · 2025-09-12T09:33:22Z

+    assert rsp.hex(' ') == '00 05 00 02 00 05'
+    # Verify version passed in dbus call is responded back
+    rsp = await bo.send_control(mctpd.network.mctp_socket, MCTPControlCommand(True, 0, 0x04, bytes([0x05])))
+    assert rsp.hex(' ') == '00 04 00 01 f4 f3 f2 f1'    


We probably don't need this in the endpoint-specific tests, right? Can it go into the more generic test_mctpd.py instead?

changed. but tests are failing after moving to test mctpd.
Will debug and fix and push

msnidhin · 2025-09-13T12:18:31Z

Tests broke after a change. Debugging

msnidhin · 2025-09-13T12:56:21Z

Tests broke after a change. Debugging

Fixed

jk-ozlabs

Looks good, just one thing inline, and addressing the CI format failure.

jk-ozlabs · 2025-09-15T03:25:23Z

    # expect new EID on D-Bus
    assert await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{second_eid}")

-


Uneeded change here

GetMsgType response is hardcoded and always return only control message support. Add a dbus method RegisterTypeSupport which accept msg type and versions array as input and reply accordingly in GetMsgType and GetVersin control commands Tested by sending GetMsgType and GetVersion from another EP Default responses for GetMsgType and GetVersion for type 0 5, 0, 1, 0 and 4, 0, 4, 241, 240, 255, 0, 241, 241, 255, 0, 241, 242, 255, 0, 241, 243, 241, 0 After calling au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 au.com.codeconstruct.MCTP1 RegisterTypeSupport yau 5 1 0xF1F2F3F4 with a tool which stays alive after method call Response for GetMsgType and GetVersion for type 5 5, 0, 2, 0, 5 and 4, 0, 1, 244, 243, 242, 241 Signed-off-by: Nidhin MS <nidhin.ms@intel.com>

jk-ozlabs · 2025-09-16T10:01:59Z

Merged, thanks!

jk-ozlabs · 2025-09-16T10:02:21Z

I have a few follow-up changes added on top of this, if you would like to review.

jk-ozlabs requested changes Jul 25, 2025

View reviewed changes

msnidhin force-pushed the get_msg_types branch from e55310e to b79fdd2 Compare August 1, 2025 08:05

vikash-chandola reviewed Sep 2, 2025

View reviewed changes

msnidhin force-pushed the get_msg_types branch 3 times, most recently from f6c4ccf to c32e360 Compare September 12, 2025 03:00

msnidhin requested review from jk-ozlabs, khangng-ampere and vikash-chandola September 12, 2025 03:00

msnidhin force-pushed the get_msg_types branch 2 times, most recently from 5c16032 to 95e82d6 Compare September 12, 2025 05:56

jk-ozlabs requested changes Sep 12, 2025

View reviewed changes

msnidhin force-pushed the get_msg_types branch from 95e82d6 to 7ab140f Compare September 13, 2025 12:15

msnidhin force-pushed the get_msg_types branch 2 times, most recently from fd0adb3 to 2dbef46 Compare September 13, 2025 12:34

msnidhin requested a review from jk-ozlabs September 13, 2025 12:56

jk-ozlabs requested changes Sep 15, 2025

View reviewed changes

msnidhin force-pushed the get_msg_types branch from 2dbef46 to 2950eda Compare September 15, 2025 04:47

msnidhin requested a review from jk-ozlabs September 15, 2025 04:49

jk-ozlabs merged commit 2950eda into CodeConstruct:main Sep 16, 2025
3 checks passed

msnidhin deleted the get_msg_types branch October 22, 2025 06:43

		# expect new EID on D-Bus
		assert await mctpd_mctp_endpoint_control_obj(dbus, f"/au/com/codeconstruct/mctp1/networks/1/endpoints/{second_eid}")

Conversation

msnidhin commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msnidhin commented Aug 1, 2025

Uh oh!

jk-ozlabs commented Aug 5, 2025

Uh oh!

msnidhin commented Aug 5, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

msnidhin commented Aug 7, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

jk-ozlabs commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msnidhin commented Sep 12, 2025

Uh oh!

msnidhin commented Sep 12, 2025

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msnidhin commented Jul 25, 2025 •

edited

Loading

jk-ozlabs Jul 25, 2025 •

edited

Loading

jk-ozlabs Sep 12, 2025 •

edited

Loading